Your changes looks good.

I am just wondering if this is the right component to use, or if we need a different filter component that re-assembles the popover and trigger separately.
As in the mockups I can see elements that look more like a shadcn dropdown menu.

the popover having a header and a button at the end

hierarchy

categories / sections to separate the list + using a simple button as trigger

Reviewed 5 of 7 files at r1, all commit messages.
Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @rolfheij-sil)

Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @rolfheij-sil)

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 135 at r1 (raw file):

                    'tw-opacity-0': Array.isArray(value)
                      ? !value.includes(option)
                      : !value || getOptionLabel(value) !== getOptionLabel(option),

please check if this mode still works for keyboard only selection or breaks this

@jolierabideau Thanks! :) I though about adding a isMultiSelect bool or something to the combobox, but I figured that when you assign an array to the value prop, you want multiselect, and if you assign a single value to it, you don't want multiselect. I don't know what UX wants here. For what you need, you could just use the ComboBox with a non-array as the value, right?

@Sebastian-ubs Thanks :) For the first two of your screenshots it seems reasonable to use the dropdown menu component, but for the last one (and the Type menu which is very similar in design) I think this combobox is a good candidate.

Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @Sebastian-ubs)

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 71 at r1 (raw file):

I don't understand what you mean by another functional parameter that will define the label based on the array input

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 135 at r1 (raw file):

Yep, works perfectly :)

Partially agree.
The mock up shows

• the trigger being a button with icon and static text and no arrows
• a headline list entry
  Which I can't see in the combobox.

Although the mock up looks different it states "filter by type of resource", which works well with acombobox.
Just a quick confirmation with Alex/Allison could be helpful.

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @rolfheij-sil)

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 71 at r1 (raw file):

a new parameter of type function e.g. "getButtonLabel" (and rename the internal func). So that consumers could implement e.g. a function that shows "Checks (5 selected)".
But maybe even more options are needed, like stated above.

Thanks for the feedback. I added three new props:

• a prop that lets you hide the Chevrons
• a prop that lets you select if you want the values to show up on the button or always want the placeholder to show
• a prop that lets you add an icon on the left.

I think it is still reasonable to keep all of this inside the same component, instead of having multiple very similar combobox-like components in PBR.

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @Sebastian-ubs)

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 71 at r1 (raw file):

Ah yes, now I understand. I suggest to not add this new feature right now. It's not needed for what I am currently building.

Added one more prop that lets you control how the popover menu should be aligned. By default it's center aligned, but I see it's start aligned for all the design you posted above @Sebastian-ubs

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @Sebastian-ubs)

You are right @rolfheij-sil, we should not end up with a lot of very similar components, but also adding a lot of properties to a single component increases the testing matrix and may make usage inconsistent and harder to understand for our users.
One goal of the component library is to have simple components with clearly distinct variances.
Therefore spotting a missing variant could also mean to go back to the designers and say "we do not have this right now. Are you sure we should add this new variant?"
So I suggest talking to agreeing on the component to use before building too many options in or creating similar components.

Reviewable status: 0 of 7 files reviewed, all discussions resolved

Yeah, I get what you're saying. I was thinking the other way around though: If I see a UX design, I assume that is what they want me to build, and add new component/variants to achieve that.
This could be something to talk about in the next UX+Dev meeting: a more high level discussion on how to convert a UX design into real code.

For now: Do you want to talk more about this specific component, or can we add these changes to PBR?

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @Sebastian-ubs)

Thank you Rolf. I guess the ux design might not have considered with which component this should be implemented. I know that in the past we had agreed to mostly use standard shadncn components and if deviating from it, make sure what the change is. The checks and home/open design however seem to have forgotten about this.
You might have even noticed that also I have contradicted my previous message and only remembered we want less variance later.

But maybe even more options are needed, like stated above.

So, yes it will be helpful to reiterate on this in the ux-dev meeting.
In general I want to review out components and define which to use when together with Jolie (but unfortunately did not recognize this would be needed now, so did not do it yet).

For this case I would suggest to implement the bare minimum, so that we might not match the design shown, but only the functionality and then discuss it in the ux-dev or as a follow up.
That would mean to remove the newly added

• a prop that lets you hide the Chevrons
• a prop that lets you select if you want the values to show up on the button or always want the placeholder to show
• a prop that lets you add an icon on the left.

Reviewable status: 0 of 7 files reviewed, all discussions resolved

Thanks! I'll wait with removing those (or not) until we've talked about it in the UX+Dev meeting. That'll save me some time :)

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @Sebastian-ubs)

I've finalized the implementation of the multi-select combo box. It's ready to be merged now, so that it can be used in the updated Get Resources design on 1290-v2-resources-and-home-extensions

Reviewable status: 0 of 18 files reviewed, all discussions resolved (waiting on @Sebastian-ubs)

Thank you very much. I added some non-blocking thoughts and few blocking comments

Reviewed 7 of 17 files at r5, 1 of 8 files at r6, 10 of 10 files at r7, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @rolfheij-sil)

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 4 at r7 (raw file):

import { Check, ChevronsUpDown, Star } from 'lucide-react';
import { ReactNode, useCallback, useMemo, useState } from 'react';
import { Button } from '../shadcn-ui/button';

should import from @components/shadcn-ui/button
Same for the 2 imports below

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 23 at r7 (raw file):

interface MultiSelectComboBoxProps {
  options: MultiSelectComboBoxEntry[];
  getOptionsCount?: (option: MultiSelectComboBoxEntry) => number;

should we rename those to "entries" and "getEntriesCount"?

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 53 at r7 (raw file):

  );

  const getPlaceholderText = () => {

Does this work well for all the cases? Or should we rather export this function?
So in this case definitively the user has to track selected items when they want to show something like "x types" right?

One issue I see - that was in the Filter design Figma mock ups, but not in the ones I added for hand off - is that the Placeholder should show up as placeholder text-muted, but a selection should show up in text-primary. Where currently all shows in primary.

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 88 at r7 (raw file):

          className={cn(
            'tw-w-full tw-justify-between',
            selected.length > 0 && selected.length < options.length && 'tw-border-primary',

I think this should be optional for a general multi select combobox. Could you add prop for it or should we rename this whole component to something like filter-combobox?

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 106 at r7 (raw file):

          <CommandInput placeholder={`Search ${placeholder.toLowerCase()}...`} />
          <CommandList>
            <CommandEmpty>No item found.</CommandEmpty>

This is not localizable.

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 28 at r7 (raw file):

  /**
   * The selected value(s) that the combo box currently holds. Must be shallow equal to one or more
   * of the options entries.

adjust back to no multi select

lib/platform-bible-react/src/components/shadcn-ui/badge.tsx line 32 at r7 (raw file):

function Badge({ className, variant, ...props }: BadgeProps) {
  return <div className={cn('pr-twp', badgeVariants({ variant }), className)} {...props} />;

It could be nice to include the x button already.
Might be done by adding a onClear prop that would if exists implicitly add a clear button or by explicitly stating both.

Thanks for reviewing

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Sebastian-ubs)

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 4 at r7 (raw file):

Done

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 23 at r7 (raw file):

Done

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 53 at r7 (raw file):

Done, I added to logic to have none/all selected placeholder show up muted

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 88 at r7 (raw file):

I don't know. Let's just leave it in here for now. If people don't like this, we can always refactor this out into a prop. But I don't want to overcomplicate this component with extra props without good reason

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 106 at r7 (raw file):

Can we just leave this out then? Or maybe display some icon? I don't think now is the right time to start thinking about localizing basic react components.

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 28 at r7 (raw file):

Done

lib/platform-bible-react/src/components/shadcn-ui/badge.tsx line 32 at r7 (raw file):

The work here is already suffering from a lot of scope creep, let's keep this just as basic as we can. Also we don't generally change shadcn components unless we have to.

Reviewed 9 of 9 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rolfheij-sil)

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 88 at r7 (raw file):

fine

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 106 at r7 (raw file):
the combobox is exporting a prop

  commandEmptyMessage = 'No option found',

Reviewable status: 12 of 18 files reviewed, 2 unresolved discussions (waiting on @Sebastian-ubs)

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 106 at r7 (raw file):

Done. Added the prop

Reviewed 6 of 6 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

lib/platform-bible-react/src/components/shadcn-ui/badge.tsx line 32 at r7 (raw file):

I agree, we could follow up on that later. If to change them or not is an open question still.